-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make Option[T]
sane again for pointer types
#18401
make Option[T]
sane again for pointer types
#18401
Conversation
In case someone wants to bring up Rust as a counterexample: Yes, Now if |
797bee6
to
9e9b929
Compare
Option[T]
sane again for pointer types
This breaks API that rely on This is not so much a matter of whether the current options module makes sense or not, but having the standard library as a playground for crusades makes it very difficult to use it in actual software - unfortunately, breaking changes like these are tightly coupled to bug and security fixes. This is a more and more frequently occurring issue. If you want to change the behavior of core modules in the library, why not start an alternative package instead where you can have full freedom? If your behavior has sufficient merit, people will migrate to it. |
That's an API misuse; use a Creating an pkg/options with the correct behavior wouldn't make sense either, Options is the one we have in stdlib and without this bugfix, APIs based on top of it in stdlib would be broken, eg
as they wouldn't be usable to distinguish whether a key is absent or wether it's present but with nil value; after this bugfix you can. If you want to add an |
This is your personal interpretation - I understand that you disagree, and why - if someone was writing the module today, I would even agree with your rationale - this is beside the point however: the current code however tells a different story - as it's written, this is a guarantee that it provides irrespective of what you consider to be the right behaviour, and because it's been there for a while (since before 1.0 for example), other code has come to depend on this. Also, if you feel you need the std lib subsidy for your code, you can always start an |
Having both Instead this PR fixes the regression introduced in #6253 so that it behaves like an Note that the following invariant is preserved: type A = ref object
var a: A
let a1 = option(a)
assert a1.isNone # both before and after PR
let a2 = some(a) # before PR: raise UnpackDefect, after PR: works, and a2.isSome is true |
You might call it a regression, but it's nonetheless a behavior that exists since the stable release of the language. It's not a bugfix, ie there are no crashes involved, and it's not a security fix - it's simply that you desire a different behavior to accommodate your other needs. This is why I advocate that you make your own packages outside of the standard library: there, you're free to break things and move fast - your changes only affect those that choose to upgrade, and that's it. In the standard library, a higher requirement for quality and stability applies: here, necessary changes and the good, non-breaking work of a whole community is bundled - the changes affect a much wider audience that uses the code in a different way than you, irrespective of the ideal you strive for. It's simply an unfortunate behavior that you have to live with, but can also exploit, in this case to exploit semantic guarantees and therefore simplify code that uses Option. There are many unfortunate design flaws in the standard library - this is one of the smaller ones, but it is what it is - the key when making changes is to make them in an additive manner such that the existing behaviors and guarantees remain. It takes a bit more effort, but that's it. |
@Araq ping on this; this PR has 11 upvotes and lots of people consider the module This PR is on the critical path for stdlib API's that make use of options, eg for |
We are using the guarantees that Option provides to reason about the safety of the code that uses it - the ability to reason about behavior and not having it change is an important step for writing stable and predictable code in nim in general - the fact that nil is not a valid value is a useful property for Option to have - this change in particular introduces a significant semantic regression. There will always be one more breaking change that you can introduce, which will seem critical from a constrained point of view - it's actually fine though, it's quite possible to software based on the current behavior, just not all the software you want - for some use cases, you need a different module than Option with a different behavior, or perhaps a different API on top of Option that is additive with respect to the current API, as opposed to destructive. |
A working options type that works in generic code is a rather fundamental data structure in a stdlib, without which we can't write something like Please vote as a reaction to this comment:
The worst option is to do nothing at all about this. |
if you want to introduce voting as a way of governance of the project, I suggest you write an RFC first where you detail how you plan to ensure the fairness of the vote (ie reaching a representative amount of voters, who is allowed to vote etc), what majority should be achieved and what the criteria should be for calling a vote - there's quite a bit of literature to take in on this subject, so I just listed the first things off the top of my head - otherwise, for technical matters, https://datatracker.ietf.org/doc/html/rfc7282 is a good start. |
Voting about this is futile. We need to find the best solution. My "proposal" (it needs to be refined): Introduce |
for inspiration: https://github.com/status-im/nim-stew/blob/3c91b8694e15137a81ec7db37c6c58194ec94a6a/stew/results.nim#L258 - we define Incidentally it works like this PR proposes (without special |
One more way to get the desired behavior for pointers/refs:
This, combined with a few operators like |
see also one of its usage => #19719 (review) |
ready for review |
@xflywind Does this make |
@Varriount Yeah, I think so. It means no special/surprsing case for pointer like types. |
@xflywind This breaks code unnecessarily in the worst case possible, I think. How about we add a new type like |
Ok. |
fwiw, |
Rejected, I take over. |
this PR #6253 introduced an "optimization" that made
Option
fundamentally flawed; it was even reported in review comments in that PR (refs #6253 (comment)) but this problem hasn't been addressed, till now.my PR fixes this so that
some(nil).isSome
remains true and Options remain sane.Specialing Option for SomePointer was a case of premature optimization similar to
std::vector<bool>
fiasco in C++ (https://isocpp.org/blog/2012/11/on-vectorbool + other; EDIT: interestingly, i see that #6253 (comment) made the same observation). It breaks usability of Option[T] as a way to provide a value outside of the valid range of values of T, ie, none and some(nil) should be different.example 1
for a table-like API with an Option API to retrieve a value for a key (possibly absent), and API like this could make sense:
but, without this PR, it wouldn't allow distinguishing whether
nil
is stored in a key or whether the key is absent.example 2
without this PR, we wouldn't be able to distinguish whether caller specified a nil value for b or whether no value (or none(T)) was specified.
nil
should be legal and not be conflated with "use default param".example 3
std/wrapnils has an API that returns an Option:
??.
, but can't distinguish between those 2 cases:eg: if
??.f1.x1[].bar.isNone
, we can't tell whether evaluatingf1.x1[].bar
would result in SIGSEGV or not; with this PR, we would be able to distinguish those 2 cases.example 4: breaks monad laws of
Option[T]
(associativity)nim's design suffers from the same flaw as
Optional
in java, which conflates a valid value with None, breaking monad laws.There's been numerous articles about this, eg:
here's an illustration of the broken monad law, where
a.map(f1).map(f2) != a.map(f2 o f1)
:note 1
instead of special casing Option for SomePointer, this PR introduces a
maybeOption
that depending on whether T is SomePointer, returns an Option[T] or T; this allows client code to choose whether they want to optimize out Option for SomePointer in the cases where conflating nil and none is ok, without otherwise impacting code usingOption[T]
forT: SomePointer
.note 2
It is indeed a breaking change, but so was #6253 when it was introduced, but this breaking change is a needed bugfix. Only 1 important_package was broken, which I'm fixing in PMunch/nim-optionsutils#6 (it was in fact just a 1-line test on the implementation of option:
let tmp = option(intref); check(sizeof(tmp) == sizeof(ptr int))
other languages
it seems only java has this broken behavior (Optional in java is widely reported as flawed in large part because of this); other languages either don't allow
nil
references, or allowsome(nil)
(and such thatsome(nil) != none
)Nullable!T
), but also has a way to assign a none value (see https://dlang.org/library/std/typecons/nullable.html#3); so it generalizes the optimization from nim's std/options beyond SomePointer types, but by default this optimization is not used. This is a better design in all fronts.indeed:
Some(null) == None
is also falselinks
Option[T]
should not special case ptr-like types, so that some(nil).isSome is true timotheecour/Nim#758future work
SomePointer
, or even better, using something like:when compiles(a == nil):
(oroverloadExists
fromresolveSymbol(foo(args))
andoverloadExists(foo(args))
to return symbol after overload resolution #12076) to decide instead ofSomePointer
, so that it'll auto-work for not nil typesstruct Nullable(T, T nullValue) ;
which allows specifying at CT a sentinel value treated as null, for which the optimization is ok